I think we can functional tests instead of just these,
But waiting for Arsenal/cloudserver to be merged, as it will be easier to make these tests (functional tests in backbeat rely on an image of cloudserver)

keeping unit test is good, functional test should just be an addition?

partAlreadyAtDest name is not correct : we cannot detect that a "part" is already at destination, only that there is already an object (i.e. a document in mongo with the specified key and versionId).
→ should really be objectAlreadyAtDest

....and it changes the logic a bit : when it happen (even on a single part), all successful parts must be deleted.

i.e. we cannot make a partial write of object data. To create the metadata, all parts must be known: so all of them must have been written successfully. If there is a "conflict" on any single part, it means another replicant has finished uploading parts and created the metadata document : so we must drop all the parts we wrote, and just update the object metadata if needed....

also please create followups to

• ignore (other) error if a single part has a conflict
• abort other parts upload as soon as we identify a conflict (i.e. imagine a 1000 parts object: if we have a conflict on first part, no point trying the other parts)
• consider changing the putPartData protocol to create actual MPU parts (which can be garbage-collected by lifecycle after transfer is aborted), for extra safety (not strictly related to CRR, but best to track it)

Yeah I rewrote this because I think from the start I overcomplicated it.
The flow is :
if any err or conflict : delete all written data, else we can all parts like usually.

For the follow up :

• I already created the follow up to do an early exit on multi part upload
• The error handling, I think no need for a follow up, I added it in the code by checking conflict before checking error (although maybe its a bit simplistic, it kinda raise some questions about whether we still want to act on the errors, at least logs, maybe more depending on the errors, not sure)
• Created a ticket for putPartDataMPU https://scality.atlassian.net/browse/BB-801

this whole function is not correct in case of putData(Parts) : if we receive VersionIdCollision, it means there is already a object with this versionId.

• We must not create or replace the metadata with the new data (data is immutable)
• We must immediately delete whatever we already uploaded
• The microVersionId comparison can be used only after this, to decide if we need to proceed with the metadata update (e.g. not the location, but the other fields: tags, ...) or can skip it

→ here the function is used to compute partAlreadyAtDest, which would make _getAndPutData silently "hide" the error if there is already some data at the destination BUT the metadata we are trying to replicate is newer.
→ _getAndPutData() should probably return the max/last microVersionId it received (in case of conflict) or nothing it is wrote all data successfully (i.e. no conflict)

(the logic here is what should happen on microVersionId conflict on metadata, not here on VersionIdConflict)

also in that case the x-scal-replication-content sent to cloudserver MUST not be DATA+METADATA anymore, but be "downgraded" to METADATA (to keep the existing data)

Ok I see this is a left over from some missunderstanding that I had.

This function can be completely removed, but before I do it, there is one thing i wanna confirm :

In the design, we said in case of putData collision, we would still do the putMetadata, and let cloudserver on putMetadata deal with stale microVersionId

But as you said, since with the putData error we get a microVersionId returned, it is possible for backbeat to do the comparison directly here to avoid a useless putMetadata.

So I wanna decide which thing we should do : I would prefer skipping the putMetadata in backbeat if its useless, but also I wonder if there was a specific reason (that i can't find) we went with the design of sending putMetada regardless of the result of putData ?

Ok I talked with Maël and review again, I'm removing this function and making the improvement to check the microversion id on conflict to avoid useless put metadata

we do exactly the same in all 3 branches : do we need 2 errors and different logs?

if (!err || err instanceof ...) {
    log.debug('replication succeeded for object, publishing ' +
                'replication status as COMPLETED',
                { entry: sourceEntry.getLogInfo(), err });
    this._publishReplicationStatus(sourceEntry, 'COMPLETED', { kafkaEntry, log });
}

I think thats what I had a few week ago + the logs but I got suggested a refactoring

Are you sure you want to drop log in case of conflicts on microVersionId ? Seems like something we might wanna know about in case of issue

there is a risk of creating orphans here, please create a followup:

1. new object was created → try to replicate DATA+META
2. putDataParts succeeds (no conflict)
3. before we could put metadata, another site was able to putData (same as us, not metadata → impossible to detect conflict) and putMetadata
4. putMetadata will thus fail with MicroVersionIdAlreadyStoredException/StaleMicroVersionIdException

→ in that case, data created in 2. must be deleted, i.e. call _deleteOrphans. _deleteOrphans MUST NOT be called if this was a META-only update / if we did not call putDataParts

Yeah I see, created this : https://scality.atlassian.net/browse/BB-802
Looking from afar i think it shouldn't be too hard to handle, I will address it when we merge this pr otherwise this one will never be merged

AFAIU this still does not implement what was agreed in the MPU thread: if some parts return partAlreadyAtDest and some were written by us, destLocations is a partial list, noNewDataLocations is false, and we proceed to putMetadata with only the parts we wrote → corrupted location metadata at destination. A conflict on any part means another replicant already wrote the metadata: we must delete all the parts we uploaded and downgrade to a metadata-only update. i.e. the check should be destLocations.length !== partResults.length, not === 0

Yes sorry I had not addressed comments yet when you did the review I was on cloudserver, it's updated now

the empty-string convention is one of the open topics on scality/cloudserver#6179. Is the shape settled now?

yep this is very intentional, using undefined would cause the sdk to not even send the header and would break the cascade replication. A bit fragile if you ask me but this is the current design

nit: rename both or neither; decode unqualified next to encodeMicroVersionId reads as if they operate on different things

do we need the condition here? Can't we just log the error (like all other errors) and pass it up in these case as well?
(the main difference is that we would get a ERROR log instead of INFO... and log here instead of in the continuation callback)

I think its nicer to keep, these arent unexpected errors, and they are logged already in the replication outcome.

I moved them with the if (err.ObjNotFound) above though, as its a similar situation : expected error

should still be a collision : even if we can't parse the microVersionID, we are still in the case "data already at destination", so we shoud still try to write the metadata anyway ?

(the only difference is that we can't really make the optimization of skipping the metadata updated if the microVersionId is already up to date on target)

Yes, I updated the code, this will be a situation where we still do a putMetadata.
I also moved the decoding to another place, more centralized

this is where compareMicroVersionId shines, please use the arsenal function:

• it makes the intent explicit (just comparing micro version id)
• the function should handle the corner cases itself ("null" micro version id...)
• we don't actually need to differentiate loop vs stale here

It would look something like this, i.e. we skip if we receive the destination micro version id (field present and no parse error) AND the micro version id is newer or equal at destination

const skipPutMetadata = destMvId !== undefined && compareMicroVersoinId(sourceEntry.getMicroVersionId(), destMvId) <= 0;
return cb(null, [], true, skipPutMetadata);

nit: I wonder if it is best to do the decoding here, or just pass the field "verbatim" in the part and let _getAndPutData() decode

• it would avoid useless decoding of every parts' microVersionId
• it would increase locally (
• but the code here still needs to make sure if forwards both the microVersionId "value" AND the information that the field was indeed provided

these 2 parameters are not independent, there are actually only 3 cases:

• no conflict (i.e. as before)
• conflict with metadata already up to date
• conflict but we have newer metadata which need to be replicated

not sure how best to handle this, maybe an object ("conflict") with the microVersionId : so we also improve locality by performing the microVersionId comparison (optimization) here?
→ undefined in the usual case (no conflict), don't even need the false like line 972
→ { microVersionId: ... } (possibly with the "conflict" field you already have) otherwise ; skip if compareMicroVersionId() <= 0

I updated it with a conflict object and move some logic around into a helper function as we still need it in 2 different places

this block is duplicated and not trivial (it handles putData conflict resolution) : should be written just once... does it makes sense do do this in _putMetadata, by adding it another parameter for putData conflict ?

I'm gonna refactor this but I prefer extracting a helper functoin that putting it in putMetadata.
It looks very weird to call a function with an object that's used at the top of the function so that the function itself decides whether it should conitnue or not

This is required as otherwise, in non mdOnly situation, there is no replicationContent header, and cloudserver has no way no know we are calling putMetadata in the context of a replication.

Anyways i think sending data, metadata for non mdOnly should've been added earlier.

Discuss : Could be moved into putMetada so that we don't have to call this from 2 places, and instead inline it.

But imo it's a weird pattern to add an extra param to a function, then call that function and use that extra param at the top to decide to potentially not run this function

either are fine with me.

• while I agree on the param (especially the skip(bool skip) anti-pattern), in this case it seems fine to me : the parameter is not just an indication to skip, but really an input allowing to make the decision ("details of the conflict").
• in my mind, putting it is putMetadata makes it a further from the anti-pattern : to me it is not skipping the metadata write, but just raising the abstraction level. PutMetadata's goal is to ensure the metadata is in the system, and it would gain the ability to reach the goal without actually making a write (kind of magic!) - similar in a way to how a getter could either read from the actual DB or quickly return the data from a cache.

(but either way may be better to keep _shouldSkipMetadata separate -just decide if it is called from within _putMetadata or before it- so each path can be demonstrated and tested easily)

May completely remove the null check if I end up update the compareMicroVersionId function.
Still reviewing cloudserver, haven't done it/decided yet

nit:

nit:

I would keep the name simple here : the destination prefix adds verbosity, but does not improve readability IMHO

nit: could even return the collisionResult directly